-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rework log verbosity (-vvv
)
#11758
base: main
Are you sure you want to change the base?
rework log verbosity (-vvv
)
#11758
Conversation
/// Give log messages more detailed context | ||
pub const UV_LOG_CONTEXT: &'static str = "UV_LOG_CONTEXT"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why this name? Should it be UV_LOG_TREE
and/or UV_LOG_DURATIONS
? Are these separable concepts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two modes in the code make several random format changes, and the best I could come up with to describe them all was "more context". This extra granularity seemed a bit excessive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess while we're touching it I'd rather have extra granularity with clear purpose than a catch-all flag that is mysterious. @konstin may have some thoughts, as he added that initial configuration.
Also cc @MichaReiser as I imagine we'll want a shared experience across the tools long-term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior is already different:
- no args: info
-v
: info-vv
: debug- -vvv`: trace (but Red Knot / Ruff only). Also changes the output format
- Use
KNOT_LOG
for getting non ruff/Red Knot debug information
We also strip trace from release builds.
Regarding the option. I don't have a strong opinion on its name. The main difference to me between the tree/flat layout and how we use it in Red Knot is that the tree layout not only logs messages but also includes the spans (with enter and exit events).
I'd also consider this env var to be outside our versioning policy (unlike UV_LOG
) so that we can iterate on naming design. E.g. should it be different env vars to switch between tree/flat but have a different env var that enables span logging for the flat layout? Or does the env var use some form of DSL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My overall ideal for the log levels is:
- Default: You get information about your project only, concisely, so you can actually follow it.
-v
: You get information about uv and what actions it performs, e.g. uv version, current platform, steps such as lock/sync/install. This information is understandable after read the docs, you could turn this on e.g. by default on CI.-vv
(orRUST_LOG=uv=debug
): You get debug messages. These are detailed information that expose some internals from uv, but usually written in a way that users can understand. It's the "user serviceable" level. You would runuv ... -vv ...
and share the gist when creating a bug reportRUST_LOG=...=trace
you want to get detailed, very verbose output for a specific area of uv. Requires understanding the uv codebase to interpret, targeted at developers.
I like the proposed changes in shifting us towards that, though I'd skip on exposing RUST_LOG=trace
to the CLI, some non-uv crates tend to get really noisy at that level and we've barely ever needed that information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no args: info
-v: info
So -v
doesn't do anything?
-vvv`: trace (but Red Knot / Ruff only). Also changes the output format
Why this decision?
We also strip trace from release builds.
This seems dubious to me, we do need our trace logs fairly often for debugging user issues. That said, we don't have an info
layer :)
we've barely ever needed that information
I don't know if I agree with this, I need these pretty often for debugging network problems. That said, it is noisy and ideally we'd improve our logging coverage instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So -v doesn't do anything?
It enables info
. We only show warnings by default. The ideal Red Knot output is only diagnostics or a simple "Done" if there are no diagnostics. We use the info
to explain what python version we inferred etc but that doesn't seem useful to show by default.
Why this decision?
Because we don't support UV_LOG_CONTEXT
and we've found the tree layout useful. But it has come up in the past that we want a dedicated way to switch between how the logs are represented that's independent from the verbosity
This seems dubious to me, we do need our trace logs fairly often for debugging user issues. That said, we don't have an info layer :)
Yeah, we'll see if we need it for Red Knot, but the trace level logs are extremely detailed, and we want to avoid the perf cost.
A side note. We haven't fully figured this out on our side. But that's where we're currently at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It enables info. We only show warnings by default. The ideal Red Knot output is only diagnostics or a simple "Done" if there are no diagnostics
This makes sense. I think you wrote the wrong thing in the initial post.
Because we don't support UV_LOG_CONTEXT and we've found the tree layout useful. But it has come up in the past that we want a dedicated way to switch between how the logs are represented that's independent from the verbosity
This is helpful. We don't support UV_LOG_CONTEXT
either today, it's toggled by the verbosity right now. I think it makes sense for us to move towards a separate option. Do you have opinions on how that's done or named? i.e., comments on my suggestions at the top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. I think you wrote the wrong thing in the initial post.
Oh right
Do you have opinions on how that's done or named? i.e., comments on my suggestions at the top?
Not really beyond what I mentioned in my original reply. But @sharkdp has worked with it more than I did, maybe he has some more thoughts?
Reworks how log verbosity flags work.
<no argument>
is the same, equivalent toRUST_LOG=off
-v
is the same, equivalent toRUST_LOG=uv=debug
-vv
is now equivalent toRUST_LOG=uv=trace
(previously it only enabled more log message context)-vvv
is now equivalent toRUST_LOG=trace
(previously it was equivalent to-vv
)The "more context" that
-vv
had has been moved to an orthogonal setting via an environment variable. SettingUV_LOG_CONTEXT=1
will add the extra context that-vv
did.In the future we may make these more granular as we try to use
info!/warn!
more.Fixes #1569